-
Notifications
You must be signed in to change notification settings - Fork 181
Adds pull request template #3943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for doing this. The things that are most important to me in a PR description are:
The AI stuff makes sense too. If we're going to include a checklist, I think it should include testing the changes and/or validating against the UI. And ensuring folks are writing cumulative docs. But it's a bit of a slippery slope from there. Was the code validated? Were the snippets tested? Did you explore other areas where this content could live? Were all instances updated? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a standardized pull request template for the Elastic Docs repository to streamline the contribution process and ensure consistency across submissions.
- Adds a structured PR template with Summary and Checklist sections to guide contributors
- Includes a Generative AI disclosure section to track usage of AI tools in contributions
- Provides a link to contribution guidelines for additional reference
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Top PR Review Issues - Quick ReferenceQuick scan of the most common issues found in PR reviews. Based on analysis of 300 recent PRs with 1,547 review comments (including 585 suggestion blocks). This comment was generated by Claude. Maybe it's helpful for determining what folks are generally missing or forgetting about in their PRs? Top Issues by Frequency
|
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| Describe what your PR changes or improves. | ||
| Describe the reason you are making this change. | ||
| If your PR fixes an issue, link it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make all of the instructions comments. Otherwise people will forget and we'll have super long PR descriptions of mostly instructions.
| Describe what your PR changes or improves. | |
| Describe the reason you are making this change. | |
| If your PR fixes an issue, link it here. | |
| <!-- | |
| Describe what your PR changes or improves. | |
| Describe the reason you are making this change. | |
| If your PR fixes an issue, link it here. | |
| --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like thinking about: What problem does it solve? Any tradeoffs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe the reason you are making this change.
This IMO should come from the issue and shouldn't need repeating.
What is most important here is the implementation detail (what the changes are, and optionally how do these changes solve the issue if not already captured in the issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I combined the summary section so it now appears as:
Describe what your PR changes or improves.
If your PR fixes an issue, link it here. If not, describe the reason you are making the change.
Love the idea to turn all the instructions into comments.
| 2. If you answered "Yes" to the previous question, please specify the tool(s) and model(s) used (e.g., Google Gemini, OpenAI ChatGPT-4, etc.). | ||
|
|
||
| Tool(s) and model(s) used: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. I'd comment out the instructions if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 2. If you answered "Yes" to the previous question, please specify the tool(s) and model(s) used (e.g., Google Gemini, OpenAI ChatGPT-4, etc.). | |
| Tool(s) and model(s) used: | |
| <!-- | |
| 2. If you answered "Yes" to the previous question, please specify the tool(s) and model(s) used (e.g., Google Gemini, OpenAI ChatGPT-4, etc.). | |
| --> | |
| Tool(s) and model(s) used: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure if there's much point in specifying what model or tool one used TBH, the point is signaling what information the author isn't confident about because they relied on AI to generate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to know where the data might have been ingested / processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, just not sure if a PR template is the right place for this. I think you're getting at the fact that non-public information shouldn't be used with these tools, but it's too late if the PR is already up.
Shouldn't we have rules against using certain tools if that's the case, rather than asking for information after the fact? I'm just trying to get at the real point of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we aim to avoid is merging code / docs that might have been processed from models we haven't approved — in this case, the PR gate provides a measure of liability / transparency / honesty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I think we should just bake this into our CLA 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll touch base with Legal about including this in our CLA, but in the meantime, this is the verbiage Legal is going forward with to meet our policy standards.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| -- | ||
|
|
||
| ## Reviewers and timeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table feels overly complex. I also don't really understand the review timelines still. More useful IMO is the PR author saying things like:
@person for technical accuracy in the X section and @team-docs for content and structure
or
@team please focus extra attention on the new ### Limitations section and the updated screenshots.
or open questions like:
Are there edge cases or user journeys I'm missing? Anything that might confuse readers?
Essentially being more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for de-scoping this PR to do the minimal AI legal compliance thing, and not try to cram general PR hygiene stuff into it TBH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for de-scoping this PR to do the minimal AI legal compliance thing, and not try to cram general PR hygiene stuff into it TBH
This is exactly what I'm going to do.
|
Few thoughts:
|
Sadly, we can't still do that, though I'm discussing this internally. |
|
Thank you for your feedback, everyone! I agree with Liam's comment to include only the AI disclosure requirements from Legal. We can add more to this template in the future to meet our team needs. |
Summary
Introduces a new pull request template for the Elastic Docs repository that includes required GenAI disclosure questions. The template is designed to help internal and external contributors provide clear and accurate documentation updates while ensuring transparency around AI-assisted contributions.
What’s changing
Why this change is needed
The Elastic Legal and Open Source teams have requested that all public-facing repositories include standardized questions about generative AI usage in documentation contributions. These questions help us:
Adding this template to the Elastic Docs repo also helps contributors understand our expectations upfront and aligns with the AI & Docs Guidelines we’ve developed internally.
What's needed from reviewers
Additional question
Should we also include a separate template specifically for external contributors, or is the single unified template sufficient?